RSDK-9839: Have Go module clients be webrtc aware. #4751

dgottlieb · 2025-01-28T04:17:55Z

No description provided.

benjirewis

So I think some other changes to grpc/shared_conn.go and a couple other files slipped into here from the last version I saw?

I'll probably need a brief synchronous meeting to understand what's happening with the NewSharedConnForModule function and some of the peer connection grabbing and setting. Or you can definitely bypass my review and get @nicksanford's or someone with more context. Up to you!

dgottlieb · 2025-01-31T14:13:39Z

grpc/shared_conn.go

-	peerConnMu     sync.Mutex
-	peerConn       *webrtc.PeerConnection
-	peerConnReady  <-chan struct{}
-	peerConnClosed <-chan struct{}


dgottlieb · 2025-01-31T14:13:50Z

grpc/shared_conn.go

+
+	ret.peerConn.OnTrack(func(trackRemote *webrtc.TrackRemote, rtpReceiver *webrtc.RTPReceiver) {
+		sid := trackRemote.StreamID()
+		if sid == "rtsp-1" {


@nicksanford , we need to do a real thing here. Not sure if the logic to put here is simple enough to communicate over a PR or if syncing in person would yield a better answer.

lets sync in person about this

This was my hack solution:
https://github.com/viamrobotics/rdk/pull/4737/files#diff-f789e424ac8cea76b93d85718d17db1578e00d0c5de523e5267df82bc8111381R103-R129

dgottlieb · 2025-01-31T14:20:45Z

robot/client/client.go

 	}
 	logger := rc.Logger().Sublogger(resource.RemoveRemoteName(name).ShortName())
-	return apiInfo.RPCClient(rc.backgroundCtx, &rc.conn, rc.remoteName, name, logger)
+	return apiInfo.RPCClient(rc.backgroundCtx, rc.getClientConn(), rc.remoteName, name, logger)


For @benjirewis 's edification, this is the thing we pass into the (camera) client constructor. Having this (newly added) getClientConn to return a SharedConn when (also new added) SetPeerConnection was called is what allows this "camera client.PeerConn" call to return a non-nil value.

It's certainly the intention that only module programs go through this code path (due to calling SetPeerConnection). And it's the intention that clients that never look at "clientConn.PeerConn" behave exactly the same (everything is funneled over the grpc.Dial("unix socker")).

What's probably a better change (but a bit more movement/copying/uncertainty), that we can iterate towards, is having module/module.go avoid calling "robotClient.ResourceByName". Such that we don't need to expose this RobotClient.SetPeerConnection method.

Yeah that vaguely makes sense to me.

What's probably a better change (but a bit more movement/copying/uncertainty), that we can iterate towards, is having module/module.go avoid calling "robotClient.ResourceByName".

I suppose we've been treating the module -> rdk client connection as a regular Golang SDK client, but I can see it's now diverging slightly. I don't have as much context as you, but I might push to continue to use ResourceByName unless you find that the module -> rdk gRPC client creation is more and more distinct from regular gRPC client creation. Just so we don't have to maintain two different gRPC client creation paths.

I suppose we've been treating the module -> rdk client connection as a regular Golang SDK client

Well, our other option is to use gRPC over WebRTC for the module connections in which case this would work without modifications! Once we moved to webrtc and used its video tracks, rdk client behavior started to diverge.

This patch shoves that detail of how module connections are special into the robot Client. But by just calling (or not calling) SetPeerConnection. The only reason I prefer to not litter robot client with that detail is because of the whole "it's part of the public interface" for applications. I don't think I'd have an opinion if the two kinds of clients were distinct.

dgottlieb · 2025-01-31T14:21:22Z

grpc/shared_conn.go

@@ -193,7 +232,7 @@ func (sc *SharedConn) ResetConn(conn rpc.ClientConn, moduleLogger logging.Logger
 	}

 	sc.peerConn = peerConn
-	sc.peerConnReady, sc.peerConnClosed, err = rpc.ConfigureForRenegotiation(peerConn, rpc.PeerRoleClient, sc.logger)
+	sc.peerConnReady, _, err = rpc.ConfigureForRenegotiation(peerConn, rpc.PeerRoleServer, sc.logger)


I have to double check if we need PeerRoleServer or Client. And I ought to document the consequence of this choice.

I've confirmed this is in fact needed.

dgottlieb · 2025-01-31T14:31:15Z

@benjirewis, I've redone this PR to officially be on top of what we just merged. And you're right that @nicksanford is a better candidate for the SharedConn changes.

Your PR Benji can be scoped down to the module and robot client changes. I left a more detailed note on the diff.

benjirewis

Generally LGTM % Nick's review.

benjirewis · 2025-01-31T19:12:10Z

robot/client/client.go

@@ -680,10 +688,10 @@ func (rc *RobotClient) ResourceByName(name resource.Name) (resource.Resource, er
 func (rc *RobotClient) createClient(name resource.Name) (resource.Resource, error) {
 	apiInfo, ok := resource.LookupGenericAPIRegistration(name.API)
 	if !ok || apiInfo.RPCClient == nil {
-		return grpc.NewForeignResource(name, &rc.conn), nil
+		return grpc.NewForeignResource(name, rc.getClientConn()), nil


There's a comment at the top of the getClientConn method that says: // Must be called with rc.mu in ReadLock+ mode. Are we doing that here? I assume the lock is already before we call createClient?

Yep -- need to unfold up a few lines: https://github.com/viamrobotics/rdk/pull/4751/files#diff-d6a5d93563cdb968513a4bc0246d93a5568b7233c639bfd2ba3e1d42c3925083R667

benjirewis · 2025-01-31T19:15:46Z

robot/client/client.go

 	}
 	logger := rc.Logger().Sublogger(resource.RemoveRemoteName(name).ShortName())
-	return apiInfo.RPCClient(rc.backgroundCtx, &rc.conn, rc.remoteName, name, logger)
+	return apiInfo.RPCClient(rc.backgroundCtx, rc.getClientConn(), rc.remoteName, name, logger)


Yeah that vaguely makes sense to me.

What's probably a better change (but a bit more movement/copying/uncertainty), that we can iterate towards, is having module/module.go avoid calling "robotClient.ResourceByName".

I suppose we've been treating the module -> rdk client connection as a regular Golang SDK client, but I can see it's now diverging slightly. I don't have as much context as you, but I might push to continue to use ResourceByName unless you find that the module -> rdk gRPC client creation is more and more distinct from regular gRPC client creation. Just so we don't have to maintain two different gRPC client creation paths.

nicksanford · 2025-02-03T14:09:07Z

grpc/shared_conn.go

+
+	ret.peerConn.OnTrack(func(trackRemote *webrtc.TrackRemote, rtpReceiver *webrtc.RTPReceiver) {
+		sid := trackRemote.StreamID()
+		if sid == "rtsp-1" {


lets sync in person about this

nicksanford · 2025-02-03T14:35:53Z

grpc/shared_conn.go

+
+	ret.peerConn.OnTrack(func(trackRemote *webrtc.TrackRemote, rtpReceiver *webrtc.RTPReceiver) {
+		sid := trackRemote.StreamID()
+		if sid == "rtsp-1" {


This was my hack solution:
https://github.com/viamrobotics/rdk/pull/4737/files#diff-f789e424ac8cea76b93d85718d17db1578e00d0c5de523e5267df82bc8111381R103-R129

nicksanford · 2025-02-03T14:38:59Z

robot/client/client.go

@@ -268,6 +275,7 @@ func New(ctx context.Context, address string, clientLogger logging.ZapCompatible
 		sessionsDisabled:    rOpts.disableSessions,
 		heartbeatCtx:        heartbeatCtx,
 		heartbeatCtxCancel:  heartbeatCtxCancel,
+		isModuleConnection:  rOpts.modName != "",


I don't see where isModuleConnection is used. Do we intend to use this in an upcoming PR? If so, how?

Thanks for catching that. That was a leftover from hacking that I decided not to use, but missed in the cleanup.

dgottlieb · 2025-02-04T21:24:19Z

components/camera/client.go

+	//
+	// Modules talking back to the viam-server for a camera stream should use the "short
+	// name"/`SDPTrackName` (i.e: `foo`).
+	if sc, ok := c.conn.(*grpc.SharedConn); ok && sc.IsConnectedToModule() {


The logic change for knowing when to use a full resource name

dgottlieb · 2025-02-04T21:29:19Z

grpc/shared_conn.go

+}
+
+// IsConnectedToModule returns whether this shared conn is being used to communicate with a module.
+func (sc *SharedConn) IsConnectedToModule() bool {


I created two methods for this because I didn't think it was obvious this was indeed binary? And I like to read these descriptions in the affirmative tense

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 28, 2025

dgottlieb requested a review from benjirewis January 30, 2025 19:28

benjirewis reviewed Jan 31, 2025

View reviewed changes

dgottlieb force-pushed the RSDK-9839 branch from e5b46e0 to 540a74c Compare January 31, 2025 14:13

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 31, 2025

dgottlieb commented Jan 31, 2025

View reviewed changes

dgottlieb requested a review from nicksanford January 31, 2025 14:31

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 31, 2025

benjirewis approved these changes Jan 31, 2025

View reviewed changes

nicksanford reviewed Feb 3, 2025

View reviewed changes

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 3, 2025

Address viam-server/module name mapping

6f6e058

dgottlieb force-pushed the RSDK-9839 branch from bb013f2 to 6f6e058 Compare February 4, 2025 21:23

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 4, 2025

dgottlieb commented Feb 4, 2025

View reviewed changes

copy back documentation changes

4264ef7

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 4, 2025

dgottlieb commented Feb 4, 2025

View reviewed changes

dgottlieb requested a review from nicksanford February 4, 2025 21:30

nicksanford approved these changes Feb 4, 2025

View reviewed changes

lint

6ecb06c

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 5, 2025

dgottlieb merged commit 3ed3cc1 into viamrobotics:main Feb 5, 2025
16 checks passed

dgottlieb deleted the RSDK-9839 branch February 5, 2025 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-9839: Have Go module clients be webrtc aware. #4751

RSDK-9839: Have Go module clients be webrtc aware. #4751

dgottlieb commented Jan 28, 2025

benjirewis left a comment

dgottlieb Jan 31, 2025

dgottlieb Jan 31, 2025 •

edited

Loading

nicksanford Feb 3, 2025

nicksanford Feb 3, 2025

dgottlieb Jan 31, 2025

benjirewis Jan 31, 2025

dgottlieb Jan 31, 2025 •

edited

Loading

dgottlieb Jan 31, 2025

dgottlieb Jan 31, 2025

dgottlieb commented Jan 31, 2025

benjirewis left a comment

benjirewis Jan 31, 2025

dgottlieb Jan 31, 2025

benjirewis Jan 31, 2025

nicksanford Feb 3, 2025

nicksanford Feb 3, 2025

nicksanford Feb 3, 2025

dgottlieb Feb 3, 2025

dgottlieb Feb 4, 2025 •

edited

Loading

dgottlieb Feb 4, 2025

RSDK-9839: Have Go module clients be webrtc aware. #4751

RSDK-9839: Have Go module clients be webrtc aware. #4751

Conversation

dgottlieb commented Jan 28, 2025

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgottlieb Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgottlieb Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgottlieb commented Jan 31, 2025

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgottlieb Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgottlieb Jan 31, 2025 •

edited

Loading

dgottlieb Jan 31, 2025 •

edited

Loading

dgottlieb Feb 4, 2025 •

edited

Loading